[SOT][FasterGuard] `ShapeMatchGuard` support dynamic shape #72564

gouzil · 2025-04-30T07:54:05Z

PR Category

Performance Optimization

PR Types

Performance

Description

ShapeMatchGuard 支持动态 shape

paddle-bot · 2025-04-30T07:54:10Z

你的PR提交成功，感谢你对开源项目的贡献!
请关注后续CI自动化测试结果，详情请参考Paddle-CI手册。
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Copilot

Pull Request Overview

This PR updates the ShapeMatchGuard to support dynamic shapes for performance optimizations.

Refactors the constructor to accept a vector of py::object and converts it to int64_t values.
Modifies the shape-checking logic in the guard implementation.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
paddle/fluid/pybind/sot/guards.h	Adjusted constructor and changed internal type usage.
paddle/fluid/pybind/sot/guards.cc	Updated shape checking logic to validate non-positive dimensions.

Copilot · 2025-04-30T08:39:01Z

paddle/fluid/pybind/sot/guards.cc

@@ -141,7 +141,7 @@ bool ShapeMatchGuard::check(PyObject* value) {
    return false;
  }
  for (size_t i = 0; i < shape.size(); ++i) {
-    if (expected_[i] && shape[i] != *expected_[i]) {
+    if (shape[i] <= 0 || shape[i] != expected_[i]) {


The updated condition checks for non-positive values and fails dynamic shapes that could be represented by a negative or zero value. To support dynamic shapes, consider revising the condition to allow for dynamic dimensions (e.g., checking if expected_[i] is set to a dynamic indicator such as -1 before comparing equality).

Suggested change

if (shape[i] <= 0 || shape[i] != expected_[i]) {

if (shape[i] <= 0 || (expected_[i] != -1 && shape[i] != expected_[i])) {

Copilot

Pull Request Overview

This PR adds dynamic shape support to the ShapeMatchGuard, improving performance by allowing flexible shape validation.

Change the ShapeMatchGuard constructor to accept a vector of py::object and represent dynamic dimensions with a sentinel value.
Replace the std::optional<int64_t> storage with std::vector<int64_t>, using -1 as the indicator for dynamic dimensions.
Update the shape-check logic in guards.cc accordingly.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
paddle/fluid/pybind/sot/guards.h	Updated constructor and internal representation to support dynamic shape
paddle/fluid/pybind/sot/guards.cc	Modified shape checking logic to accommodate dynamic shape representation

Copilot · 2025-05-06T08:43:42Z

paddle/fluid/pybind/sot/guards.h

+      if (py::isinstance<py::int_>(s)) {
+        expected_.push_back(s.cast<int64_t>());
+      } else {
+        expected_.push_back(-1);


[nitpick] Consider replacing the magic number -1 with a named constant (e.g., kDynamicDimension) to improve code readability and maintainability.

Suggested change

expected_.push_back(-1);

expected_.push_back(kDynamicDimension);

Copilot · 2025-05-06T08:43:43Z

paddle/fluid/pybind/sot/guards.cc

@@ -141,7 +141,8 @@ bool ShapeMatchGuard::check(PyObject* value) {
    return false;
  }
  for (size_t i = 0; i < shape.size(); ++i) {


[nitpick] Adding a brief comment clarifying that -1 is used as a sentinel for dynamic dimensions would help maintainers understand the control flow.

Suggested change

for (size_t i = 0; i < shape.size(); ++i) {

for (size_t i = 0; i < shape.size(); ++i) {

// -1 is used as a sentinel value for dynamic dimensions, allowing any positive size to match.

SigureMo · 2025-05-08T09:06:20Z

paddle/fluid/pybind/sot/guards.h

+      if (py::isinstance<py::int_>(s)) {
+        expected_.push_back(s.cast<int64_t>());
+      } else {
+        expected_.push_back(-1);


为什么把原来能够用类型保护的 optional<int64_t> 改成了没有类型保护的 int64_t？

如果觉得 shape 有传入 -1 的风险，就直接规避掉，构造函数里直接 PADDLE_ENFORCE_GE(shape[i], 0, ...)

去掉类型安全绝对是很蠢的操作，没错说的就是整个框架用 -1 表示 dynamic dim 这种操作

SigureMo · 2025-05-08T09:11:16Z

另外 NumPyArrayShapeMatchGuard 需要同步修改下，以及可以考虑下如何复用主体代码

paddle-ci-bot · 2025-05-10T03:10:05Z

Sorry to inform you that 308bdc8's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

…rd/support_dy_shape

Copilot

Pull Request Overview

This PR introduces support for dynamic shape matching in ShapeMatchGuard and NumPyArrayShapeGuard as part of performance optimizations.

Updated constructors in both ShapeMatchGuard and NumPyArrayShapeGuard to directly assign values from py::object without using std::make_optional.
Introduced a CHECK_SHAPE macro in guards.cc to centralize shape comparisons, replacing duplicate loop logic.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
paddle/fluid/pybind/sot/guards.h	Simplified constructors in ShapeMatchGuard and NumPyArrayShapeMatchGuard by using direct assignment.
paddle/fluid/pybind/sot/guards.cc	Added CHECK_SHAPE macro to reduce duplicate shape-checking logic.

Comments suppressed due to low confidence (1)

paddle/fluid/pybind/sot/guards.cc:63

[nitpick] Using a macro for shape checking can obscure control flow due to embedded return statements. Consider using an inline function to enhance readability and debugging clarity.

#define CHECK_SHAPE(expected, ndim, actual_shape) { ... }

Copilot · 2025-05-14T03:11:06Z

paddle/fluid/pybind/sot/guards.h

  explicit ShapeMatchGuard(const std::vector<py::object>& shape) {
    expected_.resize(shape.size());
    for (size_t i = 0; i < shape.size(); ++i) {
      if (py::isinstance<py::int_>(shape[i]) && shape[i].cast<int64_t>() > 0) {
-        expected_[i] = std::make_optional(shape[i].cast<int64_t>());
+        expected_[i] = shape[i].cast<int64_t>();


[nitpick] Direct assignment to a std::optional relies on its implicit construction from int64_t. Consider explicitly using std::make_optional for clarity and consistency.

Suggested change

expected_[i] = shape[i].cast<int64_t>();

expected_[i] = std::make_optional(shape[i].cast<int64_t>());

Copilot · 2025-05-14T03:11:06Z

paddle/fluid/pybind/sot/guards.h

  explicit NumPyArrayShapeMatchGuard(const std::vector<py::object>& shape) {
    expected_.resize(shape.size());
    for (size_t i = 0; i < shape.size(); ++i) {
      if (py::isinstance<py::int_>(shape[i]) && shape[i].cast<int64_t>() > 0) {
-        expected_[i] = std::make_optional(shape[i].cast<int64_t>());
+        expected_[i] = shape[i].cast<int64_t>();


[nitpick] Direct assignment to a std::optional here relies on implicit conversion. Consider explicit wrapping with std::make_optional for clarity.

Suggested change

expected_[i] = shape[i].cast<int64_t>();

expected_[i] = std::make_optional(shape[i].cast<int64_t>());

这里 copilot 的建议有问题么？

两者在性能上区别不大，就没改了，等 review 呢 qwq~

本来就不是性能差异，这里只是代码写法差异吧？显式优于隐式

SigureMo · 2025-05-16T07:47:39Z

paddle/fluid/pybind/sot/guards.h

  explicit NumPyArrayShapeMatchGuard(const std::vector<py::object>& shape) {
    expected_.resize(shape.size());
    for (size_t i = 0; i < shape.size(); ++i) {
      if (py::isinstance<py::int_>(shape[i]) && shape[i].cast<int64_t>() > 0) {
-        expected_[i] = std::make_optional(shape[i].cast<int64_t>());
+        expected_[i] = shape[i].cast<int64_t>();


这里 copilot 的建议有问题么？

SigureMo · 2025-05-16T07:48:27Z

paddle/fluid/pybind/sot/guards.cc

@@ -60,6 +60,20 @@ static inline PyObject* PyObject_CallOneArg(PyObject* func, PyObject* arg) {
    }                            \
  }

+#define CHECK_SHAPE(expected, ndim, actual_shape)        \


这里用函数不可以么？一定要用宏么？

两个类型不太一样

用模板呢？

…rd/support_dy_shape

SigureMo

LGTMeow

[SOT][FasterGuard] ShapeMatchGuard support dynamic shape

c193070

paddle-bot bot added the contributor External developers label Apr 30, 2025

gouzil requested a review from Copilot April 30, 2025 08:38

Copilot AI reviewed Apr 30, 2025

View reviewed changes

fix SymbolicInt

308bdc8

gouzil requested a review from Copilot May 6, 2025 08:43

Copilot AI reviewed May 6, 2025

View reviewed changes

gouzil requested a review from SigureMo May 8, 2025 08:13

SigureMo reviewed May 8, 2025

View reviewed changes

gouzil added 5 commits May 12, 2025 10:43

Merge branch 'develop' of github.com:gouzil/Paddle into sot/FasterGua…

20d83da

…rd/support_dy_shape

Merge branch 'develop' of github.com:gouzil/Paddle into sot/FasterGua…

4d96bab

…rd/support_dy_shape

add CheckShape

4f41b2e

rename CheckShape-> CHECK_SHAPE

8618e9b

fix ndim type error

40ca5f1

gouzil requested a review from Copilot May 14, 2025 03:10

Copilot AI reviewed May 14, 2025

View reviewed changes

gouzil requested a review from SigureMo May 14, 2025 04:38

SigureMo reviewed May 16, 2025

View reviewed changes

gouzil added 3 commits May 16, 2025 15:59

Merge branch 'develop' of github.com:gouzil/Paddle into sot/FasterGua…

1873967

…rd/support_dy_shape

move to template

aac5f47

fix windows ssize_t

bc80fbe

SigureMo approved these changes May 16, 2025

View reviewed changes

SigureMo merged commit 38b87ed into PaddlePaddle:develop May 16, 2025
49 of 50 checks passed

SigureMo deleted the sot/FasterGuard/support_dy_shape branch May 16, 2025 18:24

	if (shape[i] <= 0 \|\| shape[i] != expected_[i]) {
	if (shape[i] <= 0 \|\| (expected_[i] != -1 && shape[i] != expected_[i])) {

	expected_.push_back(-1);
	expected_.push_back(kDynamicDimension);

	for (size_t i = 0; i < shape.size(); ++i) {
	for (size_t i = 0; i < shape.size(); ++i) {
	// -1 is used as a sentinel value for dynamic dimensions, allowing any positive size to match.

	expected_[i] = shape[i].cast<int64_t>();
	expected_[i] = std::make_optional(shape[i].cast<int64_t>());

[SOT][FasterGuard] ShapeMatchGuard support dynamic shape #72564

[SOT][FasterGuard] ShapeMatchGuard support dynamic shape #72564

Uh oh!

Conversation

gouzil commented Apr 30, 2025

PR Category

PR Types

Description

Uh oh!

paddle-bot bot commented Apr 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 6, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SigureMo commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paddle-ci-bot bot commented May 10, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 14, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 16, 2025

Choose a reason for hiding this comment

Uh oh!

gouzil May 16, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 16, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 16, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 16, 2025

Choose a reason for hiding this comment

Uh oh!

gouzil May 16, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo May 16, 2025

Choose a reason for hiding this comment

Uh oh!

SigureMo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[SOT][FasterGuard] `ShapeMatchGuard` support dynamic shape #72564

[SOT][FasterGuard] `ShapeMatchGuard` support dynamic shape #72564

SigureMo May 8, 2025 •

edited

Loading

SigureMo commented May 8, 2025 •

edited

Loading